Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

serialization: remove combined spec #95

Merged
merged 1 commit into from
May 31, 2016

Conversation

philips
Copy link
Contributor

@philips philips commented May 27, 2016

The combined spec isn't really part of the image format and introduces
new objects that we don't have a schema for. Largely I think that it
should be replaced by something like the image layout.

#94
Signed-off-by: Brandon Philips brandon.philips@coreos.com

The combined spec isn't really part of the image format and introduces
new objects that we don't have a schema for. Largely I think that it
should be replaced by something like the image layout.

opencontainers#94
Signed-off-by: Brandon Philips <brandon.philips@coreos.com>
@philips
Copy link
Contributor Author

philips commented May 27, 2016

This is from a discussion with @stevvooe and @wking that is outlined in #92.

cc @vbatts @opencontainers/image-spec-maintainers

@wking
Copy link
Contributor

wking commented May 27, 2016

+1 to this (#94 is a nicer way to accomplish the same goal).

How does removing it square with backwards-compat 1? Maybe you
should be deprecating instead of removing?

@philips
Copy link
Contributor Author

philips commented May 27, 2016

@wking AFAIK this combined format is never used on the wire anywhere. It is only in docker save/load.

@wking
Copy link
Contributor

wking commented May 27, 2016

On Fri, May 27, 2016 at 12:23:05PM -0700, Brandon Philips wrote:

@wking AFAIK this combined format is never used on the wire
anywhere. It is only in docker save/load.

I'm not sure how far image-spec's compatibility goal extends ;). #94
is doing the same sort of thing, so it seems like the idea is in
scope for image-spec. Supporting something like #94 but not the old
Docker equivalent sounds like a broken backwards-compat.

Personally, I'm not really a backwards-compat-in-the-spec guy ;). As
long as the spec calls for appropriate identifiers, external tooling
can provide the backwards-compat (e.g. an archive unpacker that
recognizes and unpacks both the old Docker format and the new #94).
But I'm not sure where the image-spec maintainers stand on that sort
of thing.

@stevvooe
Copy link
Contributor

The implication here is that we lose compatibility with the docker save format.

For testing purposes, OCI (tools or here) should have a way of processing docker save files into the OCI image format, but I am split on whether or not we want to "lock down" the docker save format. The main issue being that it is an organic format, with several subtleties, making a correct specification challenging.

The format in #94 is wildly better, but we really need to leverage the existing tools for bootstrap of OCI.

I do however agree that this doesn't really belong in the image config specification.

@philips
Copy link
Contributor Author

philips commented May 27, 2016

@stevvooe Is there a library or something for parsing all of those formats in docker save/load? We can certainly put them into oci-image-tool. Would you be OK merging this PR if I filed an issue to add that as an oci-image-tool feature?

@stevvooe
Copy link
Contributor

@philips The package is here.

Would you be OK merging this PR if I filed an issue to add that as an oci-image-tool feature?

That sounds reasonable. We may want to add some language about translation to target formats for backwards compat for the packed archive in #94.

@philips
Copy link
Contributor Author

philips commented May 27, 2016

OK, I will add language to #94. Wanna LGTM this?

@stevvooe
Copy link
Contributor

LGTM

1 similar comment
@jonboulle
Copy link
Contributor

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants